Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to Docker file #2504

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

dwd
Copy link
Member

@dwd dwd commented Aug 13, 2024

This changes the Dockerfile quite radically, so that Openfire is built within Docker rather than outside of it. This should simplify building the image, and also make the results more repeatable.

In order to maximize the use of the cache while keeping the image size under control, this uses three stages:

  • The first stage locates and extracts all the POM files and any JAR files. This stage will be re-run on any change, but it short.
  • The second stage takes the output of the first stage and gathers dependencies. These will be cached, unless the output of the first stage (ie, the dependency information) changes. Then it copies in the full source, and builds it.
  • Finally, the runtime container is setup much as it was before, except that the runtime files are copied from the build stage rather than the filesystem directly.

The result is that a repeat build of the docker image now takes about two minutes, but can trivially be done on any docker platform (even without Java installed locally).

Notes:

  • The build stage should be able to run the mvn package in offline mode, but maven (being maven) wants to download more during this stage.
  • The .dockerignore file has of course been changed, but someone who understands Java better than I might well improve it further.

This changes the Dockerfile quite radically, so that
Openfire is built within Docker rather than outside
of it. This should simplify building the image, and
also make the results more repeatable.

In order to maximize the use of the cache while
keeping the image size under control, this
uses three stages.

The first stage locates and extracts all the POM
files and any JAR files. This stage will be re-run on
any change, but it short.

The second stage takes the output of the first stage
and gathers dependencies. These will be cached, unless
the output of the first stage (ie, the dependency
information) changes.

Then it copies in the full source, and builds it.

Finally, the runtime container is setup much as it
was before, except that the runtime files are copied
from the build stage rather than the filesystem
directly.

The result is that a repeat build of the docker image
now takes about two minutes, but can trivially be done
on any docker platform (even without Java installed
locally).

Notes:
* The build stage should be able to run the `mvn package` in offline mode, but maven (being maven) wants to download more during this stage.
* The `.dockerignore` file has of course been changed, but someone who understands Java better than I might well improve it further.
@guusdk
Copy link
Member

guusdk commented Aug 13, 2024

Thanks for this! I'm not Docker-savvy enough to review this properly. @Fishbowler @Fank can you have a look please?

Assuming that this builds Openfire from source, it may be able to leverage the Maven wrapper that's part of the repository (./mvnw) rather than download/install one.

@Fishbowler Fishbowler self-requested a review August 13, 2024 16:50
Copy link
Member

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the one thing on Maven, this looks really good!

First run: 534.9s
Second run: 0.9s 🎉

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@Fishbowler
Copy link
Member

Oooh, another experiential one.
When the JDK base image updates, it invalidates the docker cache at the first line.
Wonder if we could pin stuff for the early steps, then use latest for the later stuff? Or better to pin all the way down?

@dwd
Copy link
Member Author

dwd commented Aug 25, 2024

Oooh, another experiential one. When the JDK base image updates, it invalidates the docker cache at the first line. Wonder if we could pin stuff for the early steps, then use latest for the later stuff? Or better to pin all the way down?

I wouldn't think you'd want to try and avoid the hit there - once you start running Java, I think you want the latest possible. You could use something else for the extraction stage, but that would end up dominated by fetching the container anyway, which is why I chose to use the same one throughout.

@dwd
Copy link
Member Author

dwd commented Aug 25, 2024

Other than the one thing on Maven, this looks really good!

First run: 534.9s Second run: 0.9s 🎉

That looks like full cache. The better test is an arbitrary change (comment will do) in a source file, which is more typical developer experience.

I imagine we could make things faster by examining which packages are changed most frequently, and see if we could cache the build for some fo the less frequently changed ones, but that seems quite complex.

@dwd
Copy link
Member Author

dwd commented Aug 25, 2024

Summary of those additional commits:

  • Merged main
  • Switch to our mvnw
  • Used eclipse-temurin:17 as the base/JDK image
  • Improved caching of dependencies
  • Used a skeleton stage to improve readability/image size/parallelization
  • Added docker build to workflow

TODO list, all can/should be done post-merge:

  • If we added suitable secrets to GH, then the built image can be conditionally pushed to Docker Hub (or elsewhere)
  • Modern method for Java docker images seems to be jlink, but this needs source support I think.

@guusdk
Copy link
Member

guusdk commented Aug 26, 2024

If we added suitable secrets to GH, then the built image can be conditionally pushed to Docker Hub (or elsewhere)

We could consider publishing built images via the GitHub Packages system, which can act as a Docker registry. (That doesn't seem to require additional secrets). We've experimented recently with that in this project: https://github.com/XMPP-Interop-Testing/smack-sint-server-extensions/blob/main/.github/workflows/docker.yml

@guusdk guusdk requested a review from Fishbowler August 29, 2024 08:00
@Fishbowler
Copy link
Member

The more recent changes introduced a new problem:

> docker run --rm -it openfire:latest -demoboot
Initializing /var/lib/openfire...
chown: invalid group: ‘openfire:openfire’

@Fishbowler
Copy link
Member

I've popped on another commit for you to look at - it's got the groups, and moves the sudo install down to the last container, as its needed for the entrypoint. I don't think there's a way to move this any earlier, unless we want to layer it?

echo "is_publishable_branch=true" >> $GITHUB_OUTPUT
if [[ ${{ github.ref }} == 'refs/heads/main' ]]; then
echo "is_publishable_branch=true" >> "${GITHUB_OUTPUT}"
echo "branch_tag=latest" >> "${GITHUB_OUTPUT}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we want latest pointing at the tip of main? We consider it unstable.
Would we be better having main be main or bleeding_edge or unstable or testing or something, and work out how to match the latest release with latest?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's now "development".

@stokito
Copy link
Contributor

stokito commented Oct 18, 2024

I don't see why we need to build the Openfire in a Docker itself. It doesn't need to compile something specific for some specific platform. For the Java apps everything should be without any problems.

Ideally the whole Dockerfile should be:

FROM openjdk:11-jre
RUN apt install openfire

@Fishbowler
Copy link
Member

@stokito apt install openfire assumes that you only wish to install a version of Openfire that exists in a public repository. I feel that might not work for the number of forks that exist, let alone individual developer workflows. I for one regularly test PRs in containers.

@dwd Do you have time to cast some stink eye over the mess I've made of your PR, and of my comments?

dwd added 4 commits November 11, 2024 10:29
The backticks are a deprecated bishism
My word this is broken in Maven.
Or, indeed, at all.
@dwd
Copy link
Member Author

dwd commented Nov 11, 2024

Assuming that this builds Openfire from source, it may be able to leverage the Maven wrapper that's part of the repository (./mvnw) rather than download/install one.

Done!

@dwd
Copy link
Member Author

dwd commented Nov 11, 2024

@dwd Do you have time to cast some stink eye over the mess I've made of your PR, and of my comments?

Also done, removed the need for sudo entirely now.

@dwd
Copy link
Member Author

dwd commented Nov 11, 2024

The more recent changes introduced a new problem:

> docker run --rm -it openfire:latest -demoboot
Initializing /var/lib/openfire...
chown: invalid group: ‘openfire:openfire’

I believe this now works.

@dwd
Copy link
Member Author

dwd commented Nov 11, 2024

I'll resolve the conflicts shortly.

mvnw Show resolved Hide resolved
@Fishbowler
Copy link
Member

Works on my machine ❤️
Time to change that maintainer label?

@guusdk
Copy link
Member

guusdk commented Nov 12, 2024

Time to change that maintainer label?

Given that @Fank seems to have moved on to greener pastures, and most of the original content now has been replaced, that seems reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants